Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The OIFS Scheme #1306

Conversation

Kamyarmvn
Copy link
Contributor

Added the OIFS time scheme functionality.

@njansson njansson enabled auto-merge (squash) June 12, 2024 08:08
Copy link
Collaborator

@njansson njansson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good,

A guess a question for a wider audience e.g. @timfelle @timofeymukha @MartinKarp is if we would like to have a new fluid_pnpn for oifs or a if statement?

examples/tgv/tgv.case Outdated Show resolved Hide resolved
examples/tgv/tgv.case Outdated Show resolved Hide resolved
src/common/bcknd/device/cuda/makeoifs_kernel.h Outdated Show resolved Hide resolved
src/common/bcknd/device/hip/makeoifs_kernel.h Outdated Show resolved Hide resolved
@njansson njansson added the enhancement New feature or request label Jun 12, 2024
@njansson
Copy link
Collaborator

@timfelle Any ideas why the CICD fails, despite this having a fresh sync with develop

@timfelle
Copy link
Collaborator

timfelle commented Jun 12, 2024

@timfelle Any ideas why the CICD fails, despite this having a fresh sync with develop

The error message indicate that two if statements are present in the same step. I think there might have been an issue with the automatic merger. Stand by.

Update: see PR #1307

@njansson
Copy link
Collaborator

@Kamyarmvn can you please merge with the latest develop such that CI tests will run properly

@Kamyarmvn
Copy link
Contributor Author

@Kamyarmvn can you please merge with the latest develop such that CI tests will run properly

I will try again.

For the issue of if statement in the fluid_pnpn step subroutine, one solution seems to be using a wrapper for the advection and make_rhs derived types, initilized based on the method and then called in the step subroutine.

@timfelle
Copy link
Collaborator

Device codes are failing since the .depends_device file was not updated with the new header dependencies.

auto-merge was automatically disabled June 12, 2024 09:50

Head branch was pushed to by a user without write access

@Kamyarmvn
Copy link
Contributor Author

I'm sorry for the inconvenience. the .depends_device is updated now with the new dependencies.

@njansson njansson enabled auto-merge (squash) June 12, 2024 09:59
@timfelle
Copy link
Collaborator

timfelle commented Jun 12, 2024

please add the device kernel headers to the extra_dist list in the Makefile.am
That might help the compilers to find the new kernels.

PS. I know its a lot of manual management of the build system 🙈

auto-merge was automatically disabled June 12, 2024 12:31

Head branch was pushed to by a user without write access

Copy link
Contributor Author

@Kamyarmvn Kamyarmvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel headers added.

Copy link
Contributor Author

@Kamyarmvn Kamyarmvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taylor-Green vortex case is revised.

@Kamyarmvn
Copy link
Contributor Author

I will remove the code related to the device at this stage and add the device codes later.

@njansson
Copy link
Collaborator

please add the device kernel headers to the extra_dist list in the Makefile.am That might help the compilers to find the new kernels.

PS. I know its a lot of manual management of the build system 🙈

Reminds me that I should write the device part of the contribution guide :(

Copy link
Contributor Author

@Kamyarmvn Kamyarmvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the codes related to the device. Once again, sorry for the trouble.

@timfelle
Copy link
Collaborator

please add the device kernel headers to the extra_dist list in the Makefile.am That might help the compilers to find the new kernels.

PS. I know its a lot of manual management of the build system 🙈

Reminds me that I should write the device part of the contribution guide :(

Sorry for the reminder 😅

examples/tgv/tgv.case Outdated Show resolved Hide resolved
src/math/operators.f90 Outdated Show resolved Hide resolved
src/math/operators.f90 Outdated Show resolved Hide resolved
@timfelle
Copy link
Collaborator

@njansson I am not sure what goes wrong in the Reframe tests. Could you have a look?

@njansson
Copy link
Collaborator

@njansson I am not sure what goes wrong in the Reframe tests. Could you have a look?

Reframe seems to be behaving as expected. Seems like the scalar interface is missing arguments.

I clone the branched and gave the RBC case try (similar to the case that fails)

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x13d93f925ec0)
    frame #0: 0x000000010014fb48 neko`__scalar_pnpn_MOD_scalar_pnpn_init at scalar_pnpn.f90:227:83
   224
   225 	    call advection_factory(this%adv, params, this%c_Xh, &
   226 	                           this%chkp%ulag, this%chkp%vlag, this%chkp%wlag, &
-> 227 	                           this%chkp%dtlag, this%chkp%tlag, time_scheme, this%slag)
   228 	  end subroutine scalar_pnpn_init
   229
   230 	  !> I envision the arguments to this func might need to be expanded
Target 0: (neko) stopped.
warning: This version of LLDB has no plugin for the language "fortran90". Inspection of frame variables will be limited.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x13d93f925ec0)
  * frame #0: 0x000000010014fb48 neko`__scalar_pnpn_MOD_scalar_pnpn_init at scalar_pnpn.f90:227:83
    frame #1: 0x00000001000f9d5c neko`__case_MOD_case_init_common at case.f90:255:59
    frame #2: 0x00000001000fb88c neko`__case_MOD_case_init_from_file at case.f90:121:28
    frame #3: 0x0000000100161fb4 neko`__neko_MOD_neko_init at neko.f90:261:35
    frame #4: 0x0000000100003670 neko`MAIN__ at usr_driver.f90:7:19
    frame #5: 0x0000000100164030 neko`main at usr_driver.f90:2:6
    frame #6: 0x0000000191303f28 dyld`start + 2236
(lldb)

@Shiyu-Sandy-Du
Copy link
Collaborator

For your first point of flint failure regarding codes from others, my suggestion is to merge the latest develop branch into your own branch.

As for the second point of .depend file, you could use makedepf90 tool to generate the dependency file. Please see neko contributing instructions

@Kamyarmvn
Copy link
Contributor Author

For your first point of flint failure regarding codes from others, my suggestion is to merge the latest develop branch into your own branch.

As for the second point of .depend file, you could use makedepf90 tool to generate the dependency file. Please see neko contributing instructions

Thank you for the help. I make sure that my branch is updated before pushing the changes. I will take a closer look to see if I can find a solution.

@timofeymukha
Copy link
Collaborator

@timfelle Here is a fun corner case

missing-spaces-around-=
+---------+--------+------+------------------------------------------------------------+
| line_no | column | span |                            line                            |
+---------+--------+------+------------------------------------------------------------+
|   252   |   32   |  2   |                 if (bc_label(1:2) .eq. 'd=') then          |
|   276   |   32   |  2   |                 if (bc_label(1:2) .eq. 'n=') then          |

auto-merge was automatically disabled August 1, 2024 09:16

Head branch was pushed to by a user without write access

@Kamyarmvn Kamyarmvn closed this Aug 1, 2024
auto-merge was automatically disabled August 1, 2024 09:16

Pull request was closed

@Kamyarmvn Kamyarmvn force-pushed the Resolving-conflict-in-advection_fctry branch from ba65a74 to 47cafd4 Compare August 1, 2024 09:16
@Kamyarmvn
Copy link
Contributor Author

I just accidentally closed this PR. Would it be possible to open it again?

@njansson
Copy link
Collaborator

njansson commented Aug 2, 2024

I just accidentally closed this PR. Would it be possible to open it again?

You should be able to reopen it since it is from your repo

@Kamyarmvn Kamyarmvn reopened this Aug 3, 2024
@Kamyarmvn
Copy link
Contributor Author

I opened the pull request again.

@timfelle
Copy link
Collaborator

timfelle commented Aug 12, 2024

Note a bug in the linter rules, should be fixed by PR #1422

Linting failed for 
	src/common/bcknd/sx/rhs_maker_sx.f90

issing-space-before-parenthesis
+---------+--------+------+-----------------------------------------------+
| line_no | column | span |                      line                     |
+---------+--------+------+-----------------------------------------------+
|   239   |   38   |  3   |      real(kind=rp), intent(inout) :: phis(n)  |
|   243   |   27   |  3   |        bfs(i) = bfs(i) + phis(i) * (rho / dt) |
+---------+--------+------+-----------------------------------------------+

@Kamyarmvn
Copy link
Contributor Author

Kamyarmvn commented Aug 18, 2024

I changed variable names so that there is no error in this particular case from linter.

@Kamyarmvn
Copy link
Contributor Author

I am sorry for all the problems. It would be great if someone could help me with the flint. I get different errors from flinter on my local machine than the ones on github. For example, for rhs_maker_sx, on my local machine, I get the "Lines too long" error although the lines are not over 80 characters at all. Here I get the "scope not specified" error (I didn't change this part of the code). I also get errors for fld_file.f90 which I haven't applied any changes to as far as I remember. For case.f90 I get "incorrect-spaces-before-separator" and "incorrect-spaces-after-separator" and I don't get what is wrong here. If I remove spaces before and after "::", I still have the same error in my local machine.

@njansson njansson enabled auto-merge (squash) August 20, 2024 19:19
auto-merge was automatically disabled August 20, 2024 20:12

Head branch was pushed to by a user without write access

@njansson njansson enabled auto-merge (squash) August 21, 2024 06:19
src/math/bcknd/xsmm/opr_xsmm.F90 Outdated Show resolved Hide resolved
src/math/bcknd/sx/opr_sx.f90 Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get flinter error for: if (pe_rank .ne. 0) allocate(character(len = integer_val) :: json_buffer). It seems that it requires an space between the separators (: :) which is wrong. Is there any way this can be solved?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you synced with a recent develop? There has been some fixes for the linter that addressed similar issues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the branch is up to date.
Also, should I use submodules for opr_cpu subroutines?

auto-merge was automatically disabled August 24, 2024 17:00

Head branch was pushed to by a user without write access

@Kamyarmvn Kamyarmvn closed this Aug 24, 2024
auto-merge was automatically disabled August 24, 2024 17:00

Pull request was closed

@Kamyarmvn Kamyarmvn force-pushed the Resolving-conflict-in-advection_fctry branch from d84d7ab to ebb8e74 Compare August 24, 2024 17:00
@Kamyarmvn Kamyarmvn reopened this Aug 27, 2024
@Kamyarmvn
Copy link
Contributor Author

I tried using submodules and it was working but then it would trigger the flinter on the old modules and some of them required some work in order to pass the linting rules (too many local variables,..), therefore I used the current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: 🍻 Done
Development

Successfully merging this pull request may close these issues.

OIFS time-stepping
6 participants